airbyte/_executors/python.py (3)

227-229: Fix potential User-Agent header construction failure.

The User-Agent header construction could fail if get_version() returns None. Would you consider adding a fallback to prevent request failures, wdyt?

-            headers={"User-Agent": f"PyAirbyte/{get_version()}"},
+            version = get_version() or "unknown"
+            headers={"User-Agent": f"PyAirbyte/{version}"},

---

237-238: Improve exception handling specificity.

Using a bare except Exception: is too broad and could hide important errors. Would you consider catching specific exceptions for better debugging, wdyt?

-        except Exception:
-            return None
+        except (requests.RequestException, requests.Timeout, ValueError, KeyError):
+            return None

---

271-272: Replace broad exception handling with specific cases.

The bare except Exception: could mask important errors during version compatibility checks. Would you consider handling specific exceptions to provide better error visibility, wdyt?

-        except Exception:
-            pass
+        except (ValueError, TypeError) as e:
+            warn_once(
+                f"Error parsing version compatibility for '{package_name}': {e}. "
+                f"Proceeding with installation, but compatibility cannot be guaranteed.",
+                with_stack=False,
+            )

airbyte/_executors/python.py (2)

115-122: Consider extracting package name logic to avoid duplication.

The package name determination logic is duplicated here and in get_installed_version() (lines 190-194). Would you consider extracting this to a private method to follow DRY principles, wdyt?

+    def _get_package_name(self) -> str:
+        """Get the PyPI package name for this connector."""
+        return (
+            self.metadata.pypi_package_name
+            if self.metadata and self.metadata.pypi_package_name
+            else f"airbyte-{self.name}"
+        )
+
     def install(self) -> None:
         """Install the connector in a virtual environment.
 
         After installation, the installed version will be stored in self.reported_version.
         """
-        package_name = (
-            self.metadata.pypi_package_name
-            if self.metadata and self.metadata.pypi_package_name
-            else f"airbyte-{self.name}"
-        )
+        package_name = self._get_package_name()

---

210-238: Consider adding request timeout and caching for performance.

The PyPI API call adds latency to every connector installation. Would you consider adding a caching mechanism or making the timeout configurable to improve user experience, wdyt?

+    _PYPI_CACHE: dict[str, str | None] = {}
+
     def _get_pypi_python_requirements(self, package_name: str) -> str | None:
         """Get the requires_python field from PyPI for a package.
 
         Args:
             package_name: The PyPI package name to check
 
         Returns:
             The requires_python string from PyPI, or None if unavailable
 
         Example:
             For airbyte-source-hubspot, returns "<3.12,>=3.10"
         """
         if AIRBYTE_OFFLINE_MODE:
             return None
 
+        # Check cache first
+        if package_name in self._PYPI_CACHE:
+            return self._PYPI_CACHE[package_name]
+
         try:
             url = f"https://pypi.org/pypi/{package_name}/json"
             version = get_version() or "unknown"
             response = requests.get(
                 url=url,
-                headers={"User-Agent": f"PyAirbyte/{get_version()}"},
+                headers={"User-Agent": f"PyAirbyte/{version}"},
                 timeout=10,
             )
             response.raise_for_status()
 
             data = response.json()
-            return data.get("info", {}).get("requires_python")
+            requires_python = data.get("info", {}).get("requires_python")
+            self._PYPI_CACHE[package_name] = requires_python
+            return requires_python
 
-        except Exception:
-            return None
+        except (requests.RequestException, requests.Timeout, ValueError, KeyError):
+            self._PYPI_CACHE[package_name] = None
+            return None

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: yohannj
PR: airbytehq/PyAirbyte#716
File: airbyte/logs.py:384-402
Timestamp: 2025-07-11T19:53:44.427Z
Learning: In the PyAirbyte project, when reviewing PRs, maintain clear separation of concerns. Don't suggest changes that are outside the scope of the PR's main objective, even if they would improve consistency or fix other issues. This helps with reviewing changes and potential reverts.

Learnt from: aaronsteers
PR: airbytehq/PyAirbyte#417
File: airbyte/cli.py:503-504
Timestamp: 2024-10-10T16:17:57.989Z
Learning: In the PyAirbyte project, support for Python versions earlier than 3.10 is not necessary, as the project requires Python 3.10 or newer.

Learnt from: aaronsteers
PR: airbytehq/PyAirbyte#411
File: airbyte/cli.py:26-26
Timestamp: 2024-10-11T22:05:15.550Z
Learning: In the PyAirbyte project, when reviewing Python code and encountering a TODO comment without an issue link, I should post a friendly reminder to resolve it before merging, instead of suggesting to add an issue link.

Learnt from: aaronsteers
PR: airbytehq/PyAirbyte#411
File: airbyte/cli.py:26-26
Timestamp: 2024-10-06T17:43:09.319Z
Learning: In the PyAirbyte project, when reviewing Python code and encountering a TODO comment without an issue link, I should post a friendly reminder to resolve it before merging, instead of suggesting to add an issue link.

airbyte/_util/semver.py (1)

32-41: Great compatibility checking logic!

The use of SpecifierSet and the not in operator is the correct approach for version compatibility checking. The warning message is comprehensive and user-friendly, including all the relevant context.

One small question - should we consider wrapping the SpecifierSet parsing in a try-catch block to handle potentially malformed requires_python strings from PyPI, wdyt? The packaging library might raise InvalidSpecifier exceptions for invalid version specifiers.

tests/unit_tests/test_python_executor.py (1)

141-182: Solid test coverage for the version compatibility checking!

The tests effectively cover the three main scenarios with proper mocking of sys.version_info. The incompatible version test nicely verifies both the return value and the warning message content.

One thought - would it be worth adding a test for edge cases like boundary versions (e.g., exactly 3.10.0 or 3.12.0) to ensure the specifier matching works correctly at the boundaries, wdyt?

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: yohannj
PR: airbytehq/PyAirbyte#716
File: airbyte/logs.py:384-402
Timestamp: 2025-07-11T19:53:44.427Z
Learning: In the PyAirbyte project, when reviewing PRs, maintain clear separation of concerns. Don't suggest changes that are outside the scope of the PR's main objective, even if they would improve consistency or fix other issues. This helps with reviewing changes and potential reverts.

Learnt from: aaronsteers
PR: airbytehq/PyAirbyte#417
File: airbyte/cli.py:503-504
Timestamp: 2024-10-10T16:17:57.989Z
Learning: In the PyAirbyte project, support for Python versions earlier than 3.10 is not necessary, as the project requires Python 3.10 or newer.

Learnt from: aaronsteers
PR: airbytehq/PyAirbyte#411
File: airbyte/cli.py:26-26
Timestamp: 2024-10-06T17:43:09.319Z
Learning: In the PyAirbyte project, when reviewing Python code and encountering a TODO comment without an issue link, I should post a friendly reminder to resolve it before merging, instead of suggesting to add an issue link.

Learnt from: aaronsteers
PR: airbytehq/PyAirbyte#411
File: airbyte/cli.py:26-26
Timestamp: 2024-10-11T22:05:15.550Z
Learning: In the PyAirbyte project, when reviewing Python code and encountering a TODO comment without an issue link, I should post a friendly reminder to resolve it before merging, instead of suggesting to add an issue link.

airbyte/_executors/declarative.py (1)

131-141: Version compatibility checking logic looks solid.

The _check_version_compatibility method handles the edge cases well:

• Graceful return when metadata is missing
• Fallback to f"airbyte-{self.name}" when pypi_package_name is None
• Integration with the cached PyPI requirements function and compatibility checker

One small question - should we verify that hasattr(self.metadata, "pypi_package_name") is necessary given that ConnectorMetadata should always have this attribute defined? Or is this defensive programming for potential future changes, wdyt?

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: yohannj
PR: airbytehq/PyAirbyte#716
File: airbyte/logs.py:384-402
Timestamp: 2025-07-11T19:53:44.427Z
Learning: In the PyAirbyte project, when reviewing PRs, maintain clear separation of concerns. Don't suggest changes that are outside the scope of the PR's main objective, even if they would improve consistency or fix other issues. This helps with reviewing changes and potential reverts.

Learnt from: aaronsteers
PR: airbytehq/PyAirbyte#417
File: airbyte/cli.py:503-504
Timestamp: 2024-10-10T16:17:57.989Z
Learning: In the PyAirbyte project, support for Python versions earlier than 3.10 is not necessary, as the project requires Python 3.10 or newer.

Learnt from: aaronsteers
PR: airbytehq/PyAirbyte#411
File: airbyte/cli.py:26-26
Timestamp: 2024-10-11T22:05:15.550Z
Learning: In the PyAirbyte project, when reviewing Python code and encountering a TODO comment without an issue link, I should post a friendly reminder to resolve it before merging, instead of suggesting to add an issue link.

Learnt from: aaronsteers
PR: airbytehq/PyAirbyte#411
File: airbyte/cli.py:26-26
Timestamp: 2024-10-06T17:43:09.319Z
Learning: In the PyAirbyte project, when reviewing Python code and encountering a TODO comment without an issue link, I should post a friendly reminder to resolve it before merging, instead of suggesting to add an issue link.

Learnt from: aaronsteers
PR: airbytehq/PyAirbyte#417
File: airbyte/cli.py:503-504
Timestamp: 2024-10-10T16:17:57.989Z
Learning: In the PyAirbyte project, support for Python versions earlier than 3.10 is not necessary, as the project requires Python 3.10 or newer.